Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(cmake): add required emscripten flags #5298

Merged
merged 11 commits into from
Aug 12, 2024
Merged

fix(cmake): add required emscripten flags #5298

merged 11 commits into from
Aug 12, 2024
+35 −5

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Aug 12, 2024 •
edited
Loading

Description

Putting this in for our next release. The Pyodide authors thought this was a good idea in their Discord a while back.

Suggested changelog entry:

* When compiling for WebAssembly, add the required exception flags (CMake 3.13+)

CC @hoodmane, @agriyakhetarpal

@henryiii henryiii force-pushed the henryiii/fix/emscripten_cmake branch from 3430370 to 8e331a4 Compare August 12, 2024 02:38
rwgk
rwgk approved these changes Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM although I can really only contribute a question:

How/why did the workflow here pass before -fexceptions was added?

Asking the same question from a different angle: What is fixed by adding the option? (Is that not tested here?)

It would be great if you could add a sentence or two to the PR description, to answer that question.

agriyakhetarpal
agriyakhetarpal approved these changes Aug 12, 2024
View reviewed changes
Copy link

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @henryiii! Please note the typo in the suggested CHANGELOG entry ("requried" ➡️ "required").

@henryiii
Copy link
Collaborator Author

How/why did the workflow here pass before -fexceptions was added?

I forgot to remove it from the CI, it was manually added (done now).

I believe pybind11 always requires exception handling enabled?

@henryiii
Copy link
Collaborator Author

And you can now see what happens if the flag is missing in the CI history. We are going to pretend I intended to do that. :)

@rwgk
Copy link
Collaborator

rwgk commented Aug 12, 2024

And you can now see what happens if the flag is missing in the CI history. We are going to pretend I intended to do that. :)

Is this what you expected?

  ../../../../home/runner/work/pybind11/pybind11/tests/test_buffers.py ... [  0%]
  .............Pyodide has suffered a fatal error. Please report this to the Pyodide maintainers.

I think I understand now why it worked before (d61a69b), but TBH I don't understand why the current state of this PR produces that error.

(Google has a general ban on C++ EH, with an exception for pybind11. Of course, mixups happen, and when we accidentally tried to compile pybind11 header without C++ EH enabled, we got build errors, not runtime errors.)

I believe pybind11 always requires exception handling enabled?

Yes. (Even pybind11/pytypes.h, although that almost works without C++ EH. Making that optional would be a great thing for Google, but I never got to that.)

@henryiii henryiii force-pushed the henryiii/fix/emscripten_cmake branch from 4cac5c0 to 6a3ca2d Compare August 12, 2024 15:48
@henryiii
fix(cmake): use explicit variable for is config
3a3ab40 
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
fix(cmake): go back to ALIAS target
f7951b9 
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
chore: reduce overall diff
1223a98 
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
chore: reduce overall diff
9c89f7a 
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
chore: shorten code a bit
7273ca5 
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii merged commit fc59f4e into pybind:master Aug 12, 2024
78 checks passed
@henryiii henryiii deleted the henryiii/fix/emscripten_cmake branch August 12, 2024 20:51
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 12, 2024
@hoodmane
Copy link

hoodmane commented Aug 13, 2024 •
edited
Loading

Thanks @henryiii!

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Aug 13, 2024
henryiii added a commit that referenced this pull request Aug 13, 2024
@henryiii
fix(cmake): add required emscripten flags (#5298)
d8fcfe3 
* fix(cmake): add required emscripten flags

Signed-off-by: Henry Schreiner <[email protected]>

* Update emscripten.yaml

* fix(cmake): add required emscripten flags to headers target

Signed-off-by: Henry Schreiner <[email protected]>

* fix(cmake): incorrect detection of Emscripten

Signed-off-by: Henry Schreiner <[email protected]>

* fix(cmake): allow pybind11::headers to be modified

Signed-off-by: Henry Schreiner <[email protected]>

* fix(cmake): hide a warning when building the tests standalone

Signed-off-by: Henry Schreiner <[email protected]>

* fix(cmake): use explicit variable for is config

Signed-off-by: Henry Schreiner <[email protected]>

* fix(cmake): go back to ALIAS target

Signed-off-by: Henry Schreiner <[email protected]>

* chore: reduce overall diff

Signed-off-by: Henry Schreiner <[email protected]>

* chore: reduce overall diff

Signed-off-by: Henry Schreiner <[email protected]>

* chore: shorten code a bit

Signed-off-by: Henry Schreiner <[email protected]>

---------

Signed-off-by: Henry Schreiner <[email protected]>
@agriyakhetarpal agriyakhetarpal mentioned this pull request Sep 13, 2024
Merged
1 task
@QuLogic
Copy link
Contributor

QuLogic commented Sep 27, 2024

IIUC, this only gets added to CMake, but not pybind11-config, so this gets missed out on packages that use, e.g., Meson to build. Since Meson has its own custom pybind11 lookup (all it does it call pybind11-config --includes), do you think something should be added there, it should change to use the CMake files somehow, or should pybind11-config gain some --c(xx)flags-type flag to output stuff like this for non-CMake use?

@henryiii
Copy link
Collaborator Author

This would have to be cross-compilation aware. pybind11-config can't know you are cross-compiling to Emscripten?

@QuLogic
Copy link
Contributor

QuLogic commented Sep 30, 2024

Ah, I suppose so then. It could get a --target flag, perhaps, but then there'd have to be all that logic repeated in the script. So do you think it makes more sense to place in Meson instead?

@henryiii
Copy link
Collaborator Author

henryiii commented Oct 1, 2024

@eli-schwartz Thoughts? Happy to add whatever we need here.

@agriyakhetarpal agriyakhetarpal mentioned this pull request Nov 7, 2024
Draft
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agriyakhetarpal agriyakhetarpal agriyakhetarpal approved these changes

@rwgk rwgk rwgk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@henryiii @rwgk @hoodmane @QuLogic @agriyakhetarpal